Skip to content

fix: add support for the React Native environment. #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

duyhungtnn
Copy link
Collaborator

No description provided.

@duyhungtnn duyhungtnn marked this pull request as draft April 17, 2025 08:46
@duyhungtnn duyhungtnn changed the title fix: support react native SDK fix: support the React native environment Apr 19, 2025
@duyhungtnn duyhungtnn changed the title fix: support the React native environment fix: add support for the React Native environment. Apr 19, 2025
@duyhungtnn duyhungtnn requested a review from Copilot April 19, 2025 03:00
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for the React Native environment by updating the SDK’s initialization flow and configuration along with new and updated tests for native and node contexts.

  • Adds a native entry point (src/main.native.ts) that enforces an idGenerator via assertIdGenerator.
  • Introduces a DummyPlatformModule and updates platform module selection in both node and React Native environments.
  • Updates configuration (BKTConfig) to optionally accept an idGenerator and adjusts various test and e2e files accordingly.

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
vitest-e2e-node.config.ts New node configuration with embedded SDK version.
test/main.native.spec.ts Tests for React Native environment ensuring proper idGenerator handling.
src/main.ts Updated platform module selection based on idGenerator presence for Node.
src/main.native.ts Added React Native entry point with mandatory idGenerator enforcement.
src/internal/di/PlatformModule.ts New BasePlatformModule and DummyPlatformModule implementations added.
src/BKTConfig.ts Extended configuration to optionally support idGenerator.
e2e/node/BKTClient.spec.ts End-to-end tests for Node environment with updated logic.
e2e/browser/events.spec.ts Import path updates for browser events tests.
e2e/browser/evaluations.spec.ts Import path updates for browser evaluations tests.
e2e/browser/BKTClient.spec.ts Import path updates and revised test setup for browser client tests.
Files not reviewed (1)
  • package.json: Language not supported

@duyhungtnn duyhungtnn force-pushed the feat/support-react-native-sdk branch from c5751a0 to 316f045 Compare April 21, 2025 02:53
@duyhungtnn duyhungtnn requested a review from Copilot April 21, 2025 03:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for the React Native environment by introducing a new native entry point while updating configurations and tests for multiple environments.

  • Adds a new React Native initialization file (src/main.native.ts) that enforces the presence of an idGenerator.
  • Updates configuration and DI logic to conditionally use BasePlatformModule when an idGenerator is provided.
  • Adjusts e2e tests and environment setups (both browser and node) to use environment-appropriate fetch providers and storage handling.

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
vitest-e2e.config.ts Adds browser-specific setup file to support e2e tests in happy-dom environment.
vitest-e2e-node.config.ts Introduces a new e2e config for node with proper setup files.
test/main.native.spec.ts Adds tests ensuring requiredIdGenerator behavior in the native environment.
src/main.ts Modifies component creation to conditionally use BasePlatformModule when idGenerator is set.
src/main.native.ts New file providing React Native initialization using requiredIdGenerator.
src/internal/di/PlatformModule.ts Adds BasePlatformModule implementation for environments where an idGenerator is provided.
src/BKTConfig.ts Updates configuration to optionally include an idGenerator.
e2e/**/*.ts Adjusts various tests to rely on fetchLike and conditionally clear localStorage.
e2e/environment.ts Provides functions to set fetch provider and environment flags.
e2e/BKTClient.spec.ts Updates tests to use fetchLike and remove redundant localStorage clear calls.
Files not reviewed (1)
  • package.json: Language not supported
Comments suppressed due to low confidence (2)

src/internal/di/PlatformModule.ts:10

  • [nitpick] Consider renaming '_idGenerator' to 'idGenerator' to simplify the property name, unless the underscore prefix is a deliberate convention for indicating a protected member.
protected _idGenerator: IdGenerator

e2e/events.spec.ts:177

  • [nitpick] Consider abstracting the localStorage clearing logic into a helper function to reduce repetition and improve maintainability in your test cleanup code.
if (typeof localStorage !== 'undefined') { localStorage.clear() }

@duyhungtnn duyhungtnn requested a review from Copilot April 21, 2025 03:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds React Native support by introducing a new configuration for node and native environments while updating test setups to use the environment-specific fetch providers. Key changes include:

  • Updating Vitest config files to separate browser and node setups.
  • Adding new files for React Native support including a dedicated main.native.ts and corresponding tests.
  • Changing platform module instantiation in src/main.ts to conditionally use BasePlatformModule when an idGenerator is provided.

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
vitest-e2e.config.ts Updated test setup for browser environment with setupFiles entry.
vitest-e2e-node.config.ts New configuration file for node environment tests.
test/main.native.spec.ts Added tests for idGenerator behavior in the React Native environment.
src/main.ts Modified platform module instantiation to conditionally support idGenerator.
src/main.native.ts New file to support React Native with required idGenerator checking.
src/internal/di/PlatformModule.ts Added BasePlatformModule implementation for custom idGenerator injection.
src/BKTConfig.ts Extended configuration to include an optional idGenerator property.
e2e/*.ts Updated test setup files and specs to use fetchLike and handle environment differences.
Files not reviewed (1)
  • package.json: Language not supported
Comments suppressed due to low confidence (2)

src/main.ts:31

  • [nitpick] Consider renaming 'createNodeComponent' to a more neutral name like 'createPlatformComponent' to reflect its dual behavior when using either NodePlatformModule or BasePlatformModule.
const createNodeComponent = (config: BKTConfig, user: User): Component => {

src/main.native.ts:51

  • Although the function 'requiredIdGenerator' handles undefined values, consider explicitly checking for both 'undefined' and 'null' to cover all falsy cases, ensuring robust error handling.
export function requiredIdGenerator(config: BKTConfig): IdGenerator {

expect(events4).toHaveLength(0)
if (isNodeEnvironment) {
// on the node environment, no events should be stored after destroying the client
// because its using in-memory storage
Copy link
Preview

Copilot AI Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The inline comment on the following line uses 'its' instead of "it's"; a small grammar fix would improve clarity.

Suggested change
// because its using in-memory storage
// because it's using in-memory storage

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant